Upgrade from distribute to modern version of setuptools - #7465
Conversation
413f8a1 to
43cd520
Compare
There was a problem hiding this comment.
If we can't upgrade to 2.0.5 yet, please keep the dependency pinned at 2.0.4.
There was a problem hiding this comment.
edx-ora2 requires nltk==2.0.5. I thought it would be easier to upgrade to nltk==2.0.5 by first upgrading the requirements of edx-ora2, but if we pin the dependency it will not be possible to upgrade without conflict...
There was a problem hiding this comment.
2.5 or 2.0.5? I think the issue with not pinning a dependency is that it means we can have inconsistencies across environments, and end up with unexpected failures when servers upgrade to the latest versions unexpectedly.
There was a problem hiding this comment.
Sorry, I meant 2.0.5 (I edited my answer). I totally understand if you prefer to have pinned dependencies. You will just have to remember to upgrade both dependencies from edx-ora2 and edx-platform at the same time next time you upgrade :-)
There was a problem hiding this comment.
Let's see how the test run goes, but yeah I suspect that's going to be a requirement. I'm going to have DB take a look at this pr - probably next week - since he knows more about our requirements and the workflow for upgrading.
There was a problem hiding this comment.
In general, I'd rather see the library (edx-ora2) with flexible versions, and the application (edx-platform) with fixed ones.
|
@regisb please check out https://openedx.atlassian.net/wiki/display/TE/Upgrading+Dependent+Libraries and update your pull request description with the answers to as many of the questions as apply. |
There was a problem hiding this comment.
I'm curious why you're specifically pinning setuptools at version 9.1, when the latest stable version is 14.3.1. Why not use the latest stable version?
There was a problem hiding this comment.
As mentioned in the commit message, nltk==2.0.4 is not compatible with the most recent versions of setuptools.
There was a problem hiding this comment.
Gotcha. But if we pinned nltk==2.0.5, and updated that for edx-ora2 as well, then we could use setuptools 14.3.1, right? Or are there other reasons why we can't do that?
There was a problem hiding this comment.
NLTK is now at 2.0.5 for edx-platform and edx-ora2. Can we make this PR target the latest stable version of setuptools?
|
jenkins, test this please |
|
It appears that this pull request is blocked on #6712. Debugging context is below. When running tests on this pull request with Jenkins, Searching the web for information about this error message reveals lots of people having similar trouble. It seems like new versions of |
|
Yes, I can confirm I have had issues with pytz and setuptools. Should I create a PR to update ora2 dependencies? |
|
@regisb I commented in #6712 that I've already created PRs to upgrade |
|
We could take the opportunity to upgrade to Django 1.7. Just kidding :) |
|
tagging @feanil who is working on this in the feanil/remove_distribute branch |
|
Change makes sense to me: You'll have to update My brach for reference: https://github.com/edx/edx-platform/compare/feanil/remove_distribute I also removed |
|
My PR was for testing with wheels and I don't think it is something I'll have time to work on in the near future. I think if @regisb is willing to drive this change, I'm happy to have it be merged and just rebase my stuff from there. |
|
Hi all, Unfortunately I'm on holidays so I won't have the time to do the additional changes until next Monday. I'll update my PR first thing next weekRégis Sent from my portable toaster. Please excuse the brevity. Le 3 avril 2015 16:12:35 GMT+02:00, Jesse Zoldak notifications@github.com a écrit :
|
|
IIRC (and FWIW) @cpennington had added rednose a while back for colorizing test output. |
|
Yeah, |
|
FYI rednose was just updated by @benpatterson and @clytwynec in #7626 |
f428055 to
67cd6a9
Compare
|
I updated this PR with the most recent version of pytz to see if tests pass. |
71c52e8 to
e1f0c4f
Compare
281aa38 to
3b934fc
Compare
|
Good catch! I also took the liberty to remove the distribute install section from the create-dev-env.sh script (I'm not sure if this script is used any more?). |
|
@regisb your build is failing because none of the unit test builds can run. I can replicate this locally - please investigate and fix. Thanks! |
|
Also when I ran that command, I ended up with in my working tree, which are no longer being compiled/used anymore. I'm surprised to see that, if you actually rebased on top of the latest version of master. |
|
@regisb the |
'distribute' is the ancestor of 'setuptools', and many packages require setuptools to be installed. Upgrade of MySQL-python from 1.2.4 to 1.2.5 is required because of an incompatibility with setuptools>0.7.
3b934fc to
4ef12e4
Compare
|
Tests are fixed after a rebase. |
|
I tried building another sandbox, and got the same result as before: it still has setuptools installed at version 0.6. I found another reference to an old version of setuptools in the configuration repo, and I made a PR to update it there -- I'm currently building another sandbox that uses this PR for edx-platform and that PR for configuration. Also, @regisb: it looks like setuptools version 16 just came out. As long as this PR hasn't been merged yet, can you update it to use setuptools 16, instead of 15.2? |
|
New sandbox still didn't work. After doing some more digging, I think I found the problem -- and it's a doozy. We depend on NLTK at version 2.0.5 -- in fact, we actually upgraded from 2.0.4 to 2.0.5 specifically for this setuptools upgrade. However, NLTK's installation routine will automatically download and install distribute, if it detects that distribute is not installed. [ref 1, ref 2] It will actually uninstall an up-to-date version of setuptools and install distribute instead. Because Python packaging is a mess and no one knows how to do it properly, and at one point this was the accepted best practice. We have two options. Either we could upgrade NLTK again and jump to version 3.0.2, which is a big, scary change with lots of backwards-incompatibilies, and therefore is very unlikely to happen. Or we could fork NLTK 2.0.5 and remove the @feanil: how difficult would it be for us to fork NLTK 2.0.5? |
|
Forking is easy but I'd rather not do it if we have no plan for getting back onto the version used upstream... |
|
@feanil: from my perspective, upgrading to NLTK 3 is a large effort, and one that will definitely affect courses running on edx.org. I think it's worth doing, but I don't think it's worth doing right now. On the other hand, we're really close with this distribute-to-setuptools upgrade, and I think it's worth forking NLTK to get this change in. |
|
pardon if I'm missing something, but can we just install (or re-install) setuptools once nltk is done installing? |
|
@sarina Maybe? In my testing, when I tried to do something like that, I sometimes got weird errors involving the @feanil: is that option better or worse than forking NLTK? |
|
Reintroduce post.txt? |
|
I think if we were going to do this, you would have to add it right after the NLTK requirement so that everything except for NLTK is installed using setuptools. My question in this case would be: Will NLTK still work correctly if we do this? |
|
IMHO there wouldn't be much of an improvement over the current situation if we had to re-install setuptools over nltk; we'd probably face exactly the same problems as we do right now. So I'm in favor of forking. The fork is very limited in scope and can be documented clearly, e.g: in the README. |
|
@regisb: can you elaborate on exactly which problems we're facing right now? I'm all in favor of switching from distribute to setuptools, but I'm in favor of it simply because that means that we get to benefit from bugfixes (including security fixes) since then, and because some libraries that we want to use in the future may depend on the latest features of setuptools. I'm not aware of any problems that we're facing right now by sticking with distribute, but maybe you've hit some problems that I'm not aware of. |
|
Regarding @feanil's question: I'm fairly certain that the setuptools/distribute question is only relevant during install time. Once NLTK is installed, it shouldn't care about setuptools or distribute while NLTK is being used, so I believe that it should work correctly with setuptools installed. That being said, I haven't actually tested this. |
|
@singingwolfboy I find that sometimes multiple versions of setuptools are installed concurrently. This causes weird errors, including the |
|
Ok, I'll start following #8161. |
Context: 'distribute' is the ancestor of 'setuptools', and many packages require
setuptools to be installed. Actually, some requirements of edx-platform require an old version of distribute, while others require a more recent one.
Proposed solution: migrate to a recent version of setuptools (10.1). As a consequence, Mysql-Python needs to be upgraded, too.